chore(gas_price_service): initialize v1 metadata #2288

rymnc · 2024-10-04T08:02:17Z

Note

This is PR 1/n in introducing GasPriceServiceV1 which links to v1 of the gas-price-algorithm. This PR is mainly used to start the discussion around v1 metadata and the fields we need.

Linked Issues/PRs

fixes #2286

Description

Specifies a V1Metadata that will be stored along V0Metadata wrapped within the UpdaterMetadata struct.
We use fallible conversions between the two, v0 => v1 should be possible
Constructor for AlgorithmUpdaterV1 from V1Metadata

Bonus:

Also fixed the tests that were testing initialize_algorithm to test the UninitializedTask instead--a public interface.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

rymnc · 2024-10-04T08:04:03Z

crates/services/gas_price_service/src/v1/metadata.rs

+pub struct V1Metadata {
+    // Execution
+    /// The gas price (scaled by the `gas_price_factor`) to cover the execution of the next block
+    pub new_scaled_exec_price: u64,
+    /// The lowest the algorithm allows the exec gas price to go
+    pub min_exec_gas_price: u64,
+    /// The Percentage the execution gas price will change in a single block, either increase or decrease
+    /// based on the fullness of the last L2 block. Using `u16` because it can go above 100% and
+    /// possibly over 255%
+    pub exec_gas_price_change_percent: u16,
+    /// The height of the next L2 block
+    pub l2_block_height: u32,
+    /// The threshold of gas usage above and below which the gas price will increase or decrease
+    /// This is a percentage of the total capacity of the L2 block
+    pub l2_block_fullness_threshold_percent: ClampedPercentage,
+    // DA
+    /// The gas price (scaled by the `gas_price_factor`) to cover the DA commitment of the next block
+    pub new_scaled_da_gas_price: u64,
+
+    /// Scale factor for the gas price.
+    pub gas_price_factor: NonZeroU64,
+    /// The lowest the algorithm allows the da gas price to go
+    pub min_da_gas_price: u64,
+    /// The maximum percentage that the DA portion of the gas price can change in a single block
+    ///   Using `u16` because it can go above 100% and possibly over 255%
+    pub max_da_gas_price_change_percent: u16,
+    /// The cumulative reward from the DA portion of the gas price
+    pub total_da_rewards_excess: u128,
+    /// The height of the last L2 block recorded on the DA chain
+    pub da_recorded_block_height: u32,
+    /// The cumulative cost of recording L2 blocks on the DA chain as of the last recorded block
+    pub latest_known_total_da_cost_excess: u128,
+    /// The predicted cost of recording L2 blocks on the DA chain as of the last L2 block
+    /// (This value is added on top of the `latest_known_total_da_cost` if the L2 height is higher)
+    pub projected_total_da_cost: u128,
+    /// The P component of the PID control for the DA gas price
+    pub da_p_component: i64,
+    /// The D component of the PID control for the DA gas price
+    pub da_d_component: i64,
+    /// The last profit
+    pub last_profit: i128,
+    /// The profit before last
+    pub second_to_last_profit: i128,
+    /// The latest known cost per byte for recording blocks on the DA chain
+    pub latest_da_cost_per_byte: u128,
+}


cc: @MitchTurner, if you can describe which ones we need and which ones are just being used/mutated internally by the AlgorithmUpdater please

/// Same as with V0, but we need to multiply it by the `gas_price_factor`. A constructor would help a lot here :) pub new_scaled_exec_price: u64, /// Same as with V0 (this isn't scaled, I double checked. pub min_exec_gas_price: u64, /// Same as V0 pub exec_gas_price_change_percent: u16, /// Should be confirmed with metadata sync pub l2_block_height: u32, /// Same as V0 pub l2_block_fullness_threshold_percent: ClampedPercentage, /// Should be defined in config. Should be multiplied by `gas_price_factor`. Again, constructor would be good. pub new_scaled_da_gas_price: u64, /// `100` should be good and we can hardcode it if we want. pub gas_price_factor: NonZeroU64, /// Should be set with config pub min_da_gas_price: u64, /// Should be set with config pub max_da_gas_price_change_percent: u16, /// Zero on start (saved in Metadata) pub total_da_rewards_excess: u128, /// Currently the correct value is required here on startup, i.e. we should know what we expect next from the committer and do the block before that. BUT I think we should allow it to start with None. (saved in Metadata) pub da_recorded_block_height: u32, /// Zero on start (saved in Metadata) pub latest_known_total_da_cost_excess: u128, /// Zero on start (saved in Metadata) pub projected_total_da_cost: u128, /// Set with Config pub da_p_component: i64, /// Set with Config pub da_d_component: i64, /// Zero on start (saved in Metadata) pub last_profit: i128, /// Zero on start (saved in Metadata) pub second_to_last_profit: i128, /// Zero on start (saved in Metadata) pub latest_da_cost_per_byte: u128,

this is super useful, making the required changes!

lmk if bcb1fd3 makes sense, then i will open the PR for review from others :)

I just realized that I put (saved in Metadata) on a few of them, but there are others that I missed.

what's the action here? should I change the metadata?

bump @MitchTurner

Sorry. Missed this. I just updated.

rymnc · 2024-10-04T08:09:18Z

I have the rest of the code for GasPriceServiceV1, getting them out in chunks though

…the config

MitchTurner · 2024-10-24T20:07:56Z

So. I wanted to remove the unneeded fields from the metadatas, this let me to modifying how the tests work... which led me to fixing some of the tests. I don't know if we want to keep that in this PR.

crates/services/gas_price_service/src/v0/uninitialized_task.rs

netrome

Looks mostly good to me. There are a few dangling todos that need to be addressed. Beyond that I just have a few questions and suggestions.

netrome · 2024-10-29T11:58:05Z

crates/fuel-core/src/service/adapters/gas_price_adapters.rs

+        // TODO: we need to alter fuel-core configuration to include versioned config, after which
+        // we can directly pass it into the constructor.


Should we do it now or create a follow-up?

I'm removing the TODO. We need to decide if we are going to specify like that or just switch it to the new version and never support the old version. We're tracking the work regardless.

netrome · 2024-10-29T12:00:33Z

crates/services/gas_price_service/src/ports.rs

-    pub starting_gas_price: u64,
-    pub gas_price_change_percent: u64,
-    pub gas_price_threshold_percent: u64,
+pub enum VersionSpecificConfig {


Nit: I'd prefer the old name here GasPriceServiceConfig. The fact that it's versioned is clear enough given the V0 and V1 variants, and any code which doesn't match on the version variants shouldn't have to care if it's versioned or not.

Suggested change

pub enum VersionSpecificConfig {

pub enum GasPriceServiceConfig

netrome · 2024-10-29T12:02:54Z

crates/services/gas_price_service/src/ports.rs

+        Self::V1(metadata)
+    }
+
+    /// Extract V0MetadataInitializer if it is of V0 version


What's V0MetadataInitializer?

netrome · 2024-10-29T12:04:32Z

crates/services/gas_price_service/src/v0/tests.rs

@@ -91,27 +116,28 @@ impl MetadataStorage for ErroringMetadata {
    }
 }

+fn arb_config() -> V0AlgorithmConfig {


Nit: While we already use the arb abbreviation for the arb_metadata(), I find it easier to read if we spell out the full word:

Suggested change

fn arb_config() -> V0AlgorithmConfig {

fn arbitrary_config() -> V0AlgorithmConfig {

netrome · 2024-10-29T12:07:48Z

crates/services/gas_price_service/src/ports.rs

+pub struct GasPriceServiceConfig {
+    version_specific_config: VersionSpecificConfig,
+}


What's the point of wrapping VersionSpecificCoinfig here as opposed to just having a single enum?

pub enum GasPriceServiceConfig { V0(...) V1(...) }

netrome · 2024-10-29T12:09:46Z

crates/services/gas_price_service/src/ports.rs

+    pub fn new_v1(metadata: V1AlgorithmConfig) -> Self {
+        Self::V1(metadata)
+    }


Nit: I'd complement these constructors with with From<VXAlgorithmConfig> implementations.

netrome · 2024-10-29T12:14:56Z

crates/services/gas_price_service/src/v0/tests.rs

+// GasPriceData + Modifiable + KeyValueInspect<Column = GasPriceColumn>
+impl GasPriceData for FakeGasPriceDb {
+    fn latest_height(&self) -> Option<BlockHeight> {
+        todo!()


Dangling todo. Shuld we remove this function or when is it to be implemented?

It's fine. It just doesn't get called in the scope of any of these tests, but we're still required to pass something that implements that trait.

I can change to unimplemented if that's better.

Yeah then unimplemented conveys the intention better.

crates/services/gas_price_service/src/v1/metadata.rs

AurelienFT

Nice first step towards new gas price 🚀 WP. LGTM.

netrome

Thanks for updating! 🙏

rymnc self-assigned this Oct 4, 2024

rymnc commented Oct 4, 2024

View reviewed changes

rymnc force-pushed the chore/gas-price-service-v1-metadata branch 3 times, most recently from bcb1fd3 to cfb7b1b Compare October 7, 2024 05:30

rymnc added 4 commits October 7, 2024 11:01

chore(gas_price_service): initialize v1 metadata

8297189

chore: changelog

1b7c91e

fix: broken test

56731ca

fix: move things around, introduce versioned configuration

298de5f

rymnc force-pushed the chore/gas-price-service-v1-metadata branch from cfb7b1b to 298de5f Compare October 7, 2024 05:33

rymnc and others added 12 commits October 7, 2024 11:10

fix: cyclic dep between service and uninit task

6d1a673

Merge branch 'master' into chore/gas-price-service-v1-metadata

c980dfe

Merge branch 'master' into chore/gas-price-service-v1-metadata

c04f2c5

Fix compilation

d22c9bb

Remove unused fields in metadata

48718ad

Merge branch 'master' into chore/gas-price-service-v1-metadata

67ccb14

Fix compilation more, add new field to v1 updater

9e8750f

remove unneeded fields from v1 metadata because we can get them from …

5f96e64

…the config

Remove commented code

e3af4c4

Remove more commented code

d530dd0

Use uninitialized task instead of the helper function in test

30e5bfd

Fix the other one

8a5ab27

MitchTurner marked this pull request as ready for review October 24, 2024 20:04

MitchTurner requested review from xgreenx and Dentosal as code owners October 24, 2024 20:04

MitchTurner added 3 commits October 25, 2024 11:53

Fix integration compilation

75b68c3

Create function for converting old metadata into new metadata

4419e4d

Create function for converting metadata into updater

21ea106

MitchTurner added 2 commits October 25, 2024 15:46

Add missing fields to config and metadata, include in conversions

3266a17

Fix test name

daad75e

rymnc commented Oct 28, 2024

View reviewed changes

crates/services/gas_price_service/src/v0/uninitialized_task.rs Show resolved Hide resolved

rymnc assigned MitchTurner Oct 28, 2024

MitchTurner added 4 commits October 28, 2024 17:41

Merge branch 'master' into chore/gas-price-service-v1-metadata

5689168

Fix one test

eba0dd0

Fix the other test and hopefully not break any others

c57c0ba

Lint a little

b11b09c

netrome requested changes Oct 29, 2024

View reviewed changes

Remove redundant type, simplify constructors, other PR suggestions

10b00f6

AurelienFT reviewed Oct 29, 2024

View reviewed changes

crates/services/gas_price_service/src/v1/metadata.rs Outdated Show resolved Hide resolved

crates/services/gas_price_service/src/v1/metadata.rs Show resolved Hide resolved

MitchTurner added 2 commits October 29, 2024 17:10

Clarify comment

068789e

Merge branch 'master' into chore/gas-price-service-v1-metadata

5f347ab

MitchTurner requested a review from rafal-ch as a code owner October 29, 2024 19:50

Use NonZeroU64 in config

f00b73c

AurelienFT approved these changes Oct 30, 2024

View reviewed changes

netrome approved these changes Oct 30, 2024

View reviewed changes

MitchTurner approved these changes Oct 30, 2024

View reviewed changes

MitchTurner merged commit 7605a2a into master Oct 30, 2024
38 checks passed

MitchTurner deleted the chore/gas-price-service-v1-metadata branch October 30, 2024 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gas_price_service): initialize v1 metadata #2288

chore(gas_price_service): initialize v1 metadata #2288

rymnc commented Oct 4, 2024 •

edited

Loading

rymnc Oct 4, 2024

MitchTurner Oct 4, 2024 •

edited

Loading

rymnc Oct 4, 2024

rymnc Oct 4, 2024

MitchTurner Oct 7, 2024

rymnc Oct 7, 2024

rymnc Oct 14, 2024

MitchTurner Oct 22, 2024

rymnc commented Oct 4, 2024

MitchTurner commented Oct 24, 2024

netrome left a comment

netrome Oct 29, 2024

MitchTurner Oct 29, 2024

netrome Oct 29, 2024

netrome Oct 29, 2024

netrome Oct 29, 2024

netrome Oct 29, 2024

netrome Oct 29, 2024

netrome Oct 29, 2024

MitchTurner Oct 29, 2024

MitchTurner Oct 29, 2024

netrome Oct 29, 2024

AurelienFT left a comment •

edited

Loading

netrome left a comment

		// TODO: we need to alter fuel-core configuration to include versioned config, after which
		// we can directly pass it into the constructor.

	pub enum VersionSpecificConfig {
	pub enum GasPriceServiceConfig

	fn arb_config() -> V0AlgorithmConfig {
	fn arbitrary_config() -> V0AlgorithmConfig {

chore(gas_price_service): initialize v1 metadata #2288

chore(gas_price_service): initialize v1 metadata #2288

Conversation

rymnc commented Oct 4, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

MitchTurner Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc commented Oct 4, 2024

MitchTurner commented Oct 24, 2024

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurelienFT left a comment • edited Loading

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

rymnc commented Oct 4, 2024 •

edited

Loading

MitchTurner Oct 4, 2024 •

edited

Loading

AurelienFT left a comment •

edited

Loading